-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SoE: use new AP API and naming and make APworld #2701
Conversation
also fixes test base deprecation
as suggested by beauxq
since i forgot to git add in the previous commit. good thing we squash.
for field in fields(self): | ||
option = getattr(self, field.name) | ||
if isinstance(option, (EvermizerFlag, EvermizerFlags)): | ||
flags += getattr(self, field.name).to_flag() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice before merge,
but wouldn't this be?
flags += option.to_flag()
so you don't have to getattr
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are kind of right, I saw that after git add
, but it needs a type ignore, or two separate instance() calls, or yet another type abstraction, so I decided to not bother at the moment.
Maybe if it isn't obvious why: Flag and Flags use a typing.Protocol for self and the two protocols are incompatible, so currently we'd need to specialize on each rather than use a tuple in isinstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't notice that those were mixins.
This is how I would do it:
for field in fields(self):
option = getattr(self, field.name)
if isinstance(option, (EvermizerFlag, EvermizerFlags)):
assert isinstance(option, Option)
flags += option.to_flag()
return flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. That seems like a super easy fix, but I don't understand it (and neither does pycharm 2023.2.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing pycharm doesn't understand intersection types?
mypy and pyright see the intersection types:
option: <subclass of EvermizerFlag and Option> | <subclass of EvermizerFlags and Option>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. the assert creates "typings" that fulfil the individual protocol. Took me a while to see why this works.
And yeah, pycharm seems to think those are Option now, rather than a combined pseudo type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #2724
* SoE: new file naming also fixes test base deprecation * SoE: use options_dataclass * SoE: moar typing * SoE: no more multiworld.random * SoE: replace LogicMixin by SoEPlayerLogic object * SoE: add test that rocket parts always exist * SoE: Even moar typing * SoE: can haz apworld now * SoE: pep up test naming * SoE: use self.options for trap chances * SoE: remove unused import with outdated comment * SoE: move flag and trap extraction to dataclass as suggested by beauxq * SoE: test trap option parsing and item generation
What is this fixing or adding?
How was this tested?